Bug 2037926 - landoscript: introduce file-specific version_bump scopes#1448
Bug 2037926 - landoscript: introduce file-specific version_bump scopes#1448JohanLorenzo wants to merge 2 commits into
Conversation
JohanLorenzo
changed the title
JohanLorenzo
changed the title
I'd probably do 5 before merging anything to mozilla-taskgraph, if that gets picked up anywhere in the mean time we'd break production. |
jcristau
left a comment
jcristau
left a comment
There was a problem hiding this comment.
Not exactly sure how to change it but I'm not super happy with the way this makes scope validation functions more complex, this is critical code that would ideally remain as straightforward as possible.
| expected_scopes = {f"project:releng:lando:repo:{lando_repo}"} | ||
| expected_scopes.update(f"project:releng:lando:action:{a}" for a in actions if a != "version_bump") | ||
| if "version_bump" in actions and "project:releng:lando:action:version_bump" not in scopes: | ||
| expected_scopes.update(f"project:releng:lando:action:version_bump:file:{f}" for f in (version_bump_files or [])) |
There was a problem hiding this comment.
instead of this ugly (version_bump_files or []), consider having the caller pass an empty list, or adding and version_bump_files to the if?
There was a problem hiding this comment.
I agree; this is a case where more verbose/simpler code would be beneficial now that the logic is less than trivial. I think it also has a bug in it as currently written? (I don't see any way that project:releng:lando:action:version_bump is ever in expected_scopes?)
How about something like this?
def validate_scopes(scopes, lando_repo, actions, version_bump_files=[]):
expected_scopes = {f"project:releng:lando:repo:{lando_repo}"}
for action in actions:
if action == "version_bump":
if "project:releng:lando:action:version_bump" in scopes:
# It's superfluous to add this to the expected scopes, but a bit more clear/correct to do so? This block goes away when we stop supporting this legacy scope anyways.
expected_scopes.add("project:releng:lando:action:version_bump")
else:
expected_scopes.update(f"project:releng:lando:action:version_bump:file:{f}" for f in version_bump_files)
else:
expected_scopes.add(f"project:releng:lando:action:{action}")
There was a problem hiding this comment.
with one nit, the default for version_bump_files shouldn't be [] (use () instead).
3 participants
Merge order
dev-landoscript, merge tomasterversion_bumpgrantsvalidate_scopesBug 2037926